[SPARK-30829][SQL] Define LegacyBehaviorPolicy enumeration as the common value for result change configs by xuanyuanking · Pull Request #27579 · apache/spark

xuanyuanking · 2020-02-14T09:48:13Z

What changes were proposed in this pull request?

Define a new enumeration LegacyBehaviorPolicy in SQLConf, it will be used as the common value for result change configs.

Why are the changes needed?

During API auditing for the 3.0 release, we found several new approaches that will change the results silently. For these features, we need a common three-value config.

Does this PR introduce any user-facing change?

Yes, original config spark.sql.legacy.ctePrecedence.enabled change to spark.sql.legacy.ctePrecedencePolicy.

How was this patch tested?

Existing UT.

xuanyuanking · 2020-02-14T09:55:12Z

cc @viirya @dongjoon-hyun @cloud-fan
After further thinking, it's strange in #27454 to use Option[Boolean] to present a three-value config, especially we have other similar works in the future(e.g. #27537)
In this PR, I propose LegacyBehaviorPolicy as the common config value for these similar issues. Please take a look. Thanks.

cloud-fan · 2020-02-14T10:02:21Z

yea this looks clearer, +1

viirya · 2020-02-14T10:04:16Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala


-  val LEGACY_CTE_PRECEDENCE_ENABLED = buildConf("spark.sql.legacy.ctePrecedence.enabled")
+  object LegacyBehaviorPolicy extends Enumeration {
+    val EXCEPTION, LEGACY, NEW_BEHAVIOR = Value


NEW_BEHAVIOR sounds not a good name. INNER_FIRST?

Or INNER_PRECEDENCE?

Thanks for the advice @viirya. Yes, both INNER_FIRST and INNER_PRECEDENCE are more accurate in this PR, but I also want to make this enum as a common config value, which can be reused in other similar issues. WDYT?

how about CORRECTED to indicate that it's the corrected behavior?

Thanks, change to CORRECTED in 47edd88

viirya · 2020-02-14T10:07:09Z

+1 this is better.

cloud-fan · 2020-02-14T11:52:17Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CTESubstitution.scala

-                s"Please set ${LEGACY_CTE_PRECEDENCE_ENABLED.key} to false so that name defined " +
-                "in inner CTE takes precedence. See more details in SPARK-28228.")
+                s"Please set ${LEGACY_CTE_PRECEDENCE_POLICY.key} to NEW_BEHAVIOR so that name " +
+                "defined in inner CTE takes precedence. See more details in SPARK-28228.")


since it's enum now, maybe also explain what will happen if set it to legacy.

Sure, done in 47edd88.

SparkQA · 2020-02-14T15:13:13Z

Test build #118414 has finished for PR 27579 at commit af7827c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-17T07:22:11Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

-      "false, inner CTE definitions take precedence. The default value is empty, " +
+    .doc("When LEGACY, outer CTE definitions takes precedence over inner definitions. If set to " +
+      "CORRECTED, inner CTE definitions take precedence. The default value is EXCEPTION, " +
      "AnalysisException is thrown while name conflict is detected in nested CTE.")


let's add one more sentence: This config will be removed in future versions and CORRECTED will be the only behavior.

Thanks, added in bc70f75.

cloud-fan · 2020-02-17T07:22:29Z

sql/core/src/test/resources/sql-tests/inputs/cte-nonlegacy.sql

@@ -1,2 +1,2 @@
--SET spark.sql.legacy.ctePrecedence.enabled = false
+--SET spark.sql.legacy.ctePrecedencePolicy = new_behavior 


Ah yes, done in bc70f75.

SparkQA · 2020-02-17T08:05:01Z

Test build #118542 has finished for PR 27579 at commit 47edd88.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-02-17T15:35:11Z

Test build #118565 has finished for PR 27579 at commit bc70f75.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-17T16:52:26Z

thanks, merging to master/3.0!

…mon value for result change configs ### What changes were proposed in this pull request? Define a new enumeration `LegacyBehaviorPolicy` in SQLConf, it will be used as the common value for result change configs. ### Why are the changes needed? During API auditing for the 3.0 release, we found several new approaches that will change the results silently. For these features, we need a common three-value config. ### Does this PR introduce any user-facing change? Yes, original config `spark.sql.legacy.ctePrecedence.enabled` change to `spark.sql.legacy.ctePrecedencePolicy`. ### How was this patch tested? Existing UT. Closes #27579 from xuanyuanking/SPARK-30829. Authored-by: Yuanjian Li <xyliyuanjian@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit e4a541b) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

xuanyuanking · 2020-02-18T02:09:35Z

Thanks for the review.

…l.legacy.ctePrecedencePolicy` ### What changes were proposed in this pull request? Fix the migration guide document for `spark.sql.legacy.ctePrecedence.enabled`, which is introduced in #27579. ### Why are the changes needed? The config value changed. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Document only. Closes #27782 from xuanyuanking/SPARK-30829-follow. Authored-by: Yuanjian Li <xyliyuanjian@gmail.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org>

…l.legacy.ctePrecedencePolicy` ### What changes were proposed in this pull request? Fix the migration guide document for `spark.sql.legacy.ctePrecedence.enabled`, which is introduced in #27579. ### Why are the changes needed? The config value changed. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Document only. Closes #27782 from xuanyuanking/SPARK-30829-follow. Authored-by: Yuanjian Li <xyliyuanjian@gmail.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org> (cherry picked from commit f7f1948) Signed-off-by: HyukjinKwon <gurwls223@apache.org>

…mon value for result change configs ### What changes were proposed in this pull request? Define a new enumeration `LegacyBehaviorPolicy` in SQLConf, it will be used as the common value for result change configs. ### Why are the changes needed? During API auditing for the 3.0 release, we found several new approaches that will change the results silently. For these features, we need a common three-value config. ### Does this PR introduce any user-facing change? Yes, original config `spark.sql.legacy.ctePrecedence.enabled` change to `spark.sql.legacy.ctePrecedencePolicy`. ### How was this patch tested? Existing UT. Closes apache#27579 from xuanyuanking/SPARK-30829. Authored-by: Yuanjian Li <xyliyuanjian@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…l.legacy.ctePrecedencePolicy` ### What changes were proposed in this pull request? Fix the migration guide document for `spark.sql.legacy.ctePrecedence.enabled`, which is introduced in apache#27579. ### Why are the changes needed? The config value changed. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Document only. Closes apache#27782 from xuanyuanking/SPARK-30829-follow. Authored-by: Yuanjian Li <xyliyuanjian@gmail.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org>

SPARK-30829

af7827c

viirya reviewed Feb 14, 2020

View reviewed changes

cloud-fan reviewed Feb 14, 2020

View reviewed changes

address comments

47edd88

cloud-fan reviewed Feb 17, 2020

View reviewed changes

cloud-fan approved these changes Feb 17, 2020

View reviewed changes

fix

bc70f75

cloud-fan closed this in e4a541b Feb 17, 2020

xuanyuanking deleted the SPARK-30829 branch February 18, 2020 02:09

xuanyuanking mentioned this pull request Feb 18, 2020

[SPARK-31410][SPARK-30668][SQL][FOLLOWUP] Raise exception instead of silent change for new DateFormatter #27537

Closed

xuanyuanking mentioned this pull request Mar 4, 2020

[SPARK-30289][FOLLOWUP][DOC] Update the migration guide for spark.sql.legacy.ctePrecedencePolicy #27782

Closed

		@@ -1,2 +1,2 @@
		--SET spark.sql.legacy.ctePrecedence.enabled = false
		--SET spark.sql.legacy.ctePrecedencePolicy = new_behavior

Conversation

xuanyuanking commented Feb 14, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

xuanyuanking commented Feb 14, 2020

Uh oh!

cloud-fan commented Feb 14, 2020

Uh oh!

viirya Feb 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya commented Feb 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 17, 2020

Uh oh!

SparkQA commented Feb 17, 2020

Uh oh!

cloud-fan commented Feb 17, 2020

Uh oh!

xuanyuanking commented Feb 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

viirya Feb 14, 2020 •

edited

Loading